@Kriys94

I plan on reviewing this but first I want to better understand the problems you see this controller solving and how you anticipate it being used.

In the explanation for this PR, you say that as new controllers with data subscriptions are added, clients must be updated to inform those controllers to start when the UI is active and stop when the UI is not. You say that this isn't scalable because this code is scattered throughout the clients. Furthermore, in the technical proposal you created earlier, you seem to say that the UI should not be responsible for managing data subscriptions, the controller layer should. This is further implied in this PR when suggesting that with the existence of an ApplicationStateController, other controllers can now be aware of when the UI is open and automatically start and stop data subscriptions.

First, I don't think it is bad that there is some code in the UI which controls when a data subscription is started or stopped. After all, not every screen needs every piece of data we use throughout MetaMask, so it seems perfectly fine to me to place that control at the React component level (i.e., "when this component mounts, start polling or open a WebSocket; when this components unmounts, stop").

My sense is that the problem we're trying to solve here is not how to handle what happens if a user switches away from a screen, but how to handle when the app (whether that's the mobile app, or a tab in a browser) is backgrounded or foregrounded. Here, the user is still on a screen — so no component is unmounted — but the user is not actively using the app. And in this case, it seems to me that if the current screen is actively polling for data or has a WebSocket open, then that data subscription needs to be paused, so that when the user returns, it can be resumed.

In other words, alongside this controller I wonder if we ought to update the pattern that we've been using so that polling controllers, WebSocket services, and other places have both a pause and a resume method in addition to start and stop. (resume would be like start, but the subscription would only be activated if it was previously paused; otherwise nothing would happen. And pause would merely track the state of the subscription before stopping.)

I think reframing this controller — and the examples presented in places like the README — in this way would make more sense to me, but what do you think?

First, I don't think it is bad that there is some code in the UI which controls when a data subscription is started or stopped. After all, not every screen needs every piece of data we use throughout MetaMask, so it seems perfectly fine to me to place that control at the React component level (i.e., "when this component mounts, start polling or open a WebSocket; when this components unmounts, stop").

Currently, when you look at the core codebase, you don't have a complete picture of how/when some code is executed. This is extremely hard to understand the whole workflow when you don't know that some logic lives in the UI.
This is especially useful when the application state is used in multiple places.

To me, it's the same utility as when we switch accounts: we have a state and events that we can listen to in any Controller, with no UI code/hooks needed. Thanks to this App State Event, we could consider removing all polling logic from the extension and mobile, which represents quite some code to clean, but ultimately will make platform code lighter.

.@Kriys94 and I spoke earlier today about this. For completeness here is more or less a summary of what I said:

Implications on polling controllers

To be clear, I don't think this controller is not needed. I can see the value in encapsulating the concept of whether the user is currently using MetaMask so that we can access that information in an agnostic fashion.

However, there are caveats around how this controller ought to be used that I think should be highlighted or at least addressed somewhere. Namely, when it comes to polling controllers, I don't know that we would want to suggest that engineers can update their controllers to start polling when MetaMask is active and stop polling when it's inactive, e.g.:

this.#messenger.subscribe(
  'ApplicationStateController:stateChange',
  (isClientOpen) => {
    if (isClientOpen) {
      this.#start();
    } else {
      this.#stop();
    }
  },
  (state) => state.isClientOpen
);

This pattern implies that we would be moving polling management to a more global location: as soon as MetaMask opens, no matter what screen the user is on, we start subscribing to updates for all kinds of data — even for ones that the current screen doesn't need. I don't feel like this is a good strategy long-term. We've gotten in trouble in the past about making network requests before users complete onboarding and I feel like we would run into that again if we went that direction.

That's why I suggested continuing to have the UI drive which polls are active, but then introducing the idea of "pausing" and "resuming", for instance:

this.#messenger.subscribe(
  'ApplicationStateController:stateChange',
  (isClientOpen) => {
    if (isClientOpen) {
      this.#resume();
    } else {
      this.#pause();
    }
  },
  (state) => state.isClientOpen
);

If we are providing examples for how to use this controller in this PR, then I think we should suggest that engineers follow these practices.

Naming

The other thing I mentioned in the call is around naming:

1. The extension repo already has an AppStateController, so introducing an ApplicationStateController could get confusing.
2. The state property is called isClientOpen. This could be possibly inconsistent and confusing depending on perspective.

I think the name ApplicationStateController is fine. It's a bit generic, but I can imagine having one or two other state properties that we might want to track that don't really belong anywhere else. Maybe we can plan to rename AppStateController in extension to ExtensionApplicationStateController or something, though?

I also wonder if we should rename the state property? I am fine with the word "open". On extension, it could reasonably mean that the popup is open, or the full-screen view is open (regardless of whether the tab is active), or the sidepanel is open. And on mobile, it would mean that the app is foregrounded. But, "client" seems like a synonym for "application" to me. So maybe we should rename the state property either isApplicationOpen or isUiOpen?

@mcmire For the naming, I called it ClientStateController. WDYT?

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request